Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build/efinix: add a few IO primitives, IO constraints, sdc rework #2060

Merged
merged 5 commits into from
Sep 10, 2024

Conversation

Dolu1990
Copy link
Collaborator

@Dolu1990 Dolu1990 commented Sep 5, 2024

add a few IO primitives, IO constraints, but mainly it rework how the SDC are handled.

The main issue with how the efinix flow was handled, is that the constraints for the syncronous IO were missing. This break stuff for high frequency design in large FPGA (Ti375)
The efinity tool generate a template outflow/xxx.pt.sdc. With this patch, this xxx.pt.sdc is combined with the one generated from litex.

!!! WARNING !!!
There is one dirty thing is this PR.
This is the changes in efinix.py. when you setup a clock domain with a pll, this replace the clock domain "clk" with the new one created by the PLL.
This is done to ensure the constraints match with the efinix pll toplevel IO.

An alternative would be to provide a way to specify that a given litex clock domain should use the given name in the SDC file (instead of the clock domain "clk" name)
Let me know what you think about this, this can be reworked.

let's me know if the PR need to be splitted.

@trabucayre
Copy link
Collaborator

PLL's modifications look good to be able to use a ClockSignal instead of a string but I see one issue: When more than one module with a PLL are added to a gateware clk_name is not unique. Maybe it's required to append name with id (PLL id ?)

@Dolu1990
Copy link
Collaborator Author

Dolu1990 commented Sep 9, 2024

Maybe it's required to append name with id (PLL id ?)

Either that, and this also mean that the clock name get some extra "noise",
Either removing this whole trick around "cd.clk = clk_out" and instead reworking the emited SDC file by replacing all occurance of cd.clk names into its PLL input pin counter part. I think this would kinda be less hack. Wanna me to try ? Could probably be done here : https://github.com/enjoy-digital/litex/pull/2060/files#diff-da6a1f772f6f2d1119b40ece412e6e70650c0bddfa9ba76ef8cd3d9887df77f5R327
It would just need a dictionary populated with all the replacement required. That dictionary could be populated in the create_clkout function.

@trabucayre
Copy link
Collaborator

I already did some try with an id. The problem is more when the script is executed to avoid conflict between signals. I'm agree this adds a bit of noise. @enjoy-digital has also propose to adds primitive type+id when a clock is created to see if it's a PLL, or clkin,...

…s input_signal name when name is not provided and PLL is configured in CORE or INTERNAL mode, create_clkout: added PLL name in clk_name str
…clk_out_name. litex/build/efinix/ifacewriter.py: generate_lvds: when slow_clk/fast_clk are ClockSignal uses platform.clks to map between domain and signal name
@trabucayre trabucayre merged commit dc8b74c into enjoy-digital:master Sep 10, 2024
1 check passed
@trabucayre
Copy link
Collaborator

Applied with few addtions. Thanks @Dolu1990 !


class EfinixDDRTristateImpl(Module):
def __init__(self, platform, io, o1, o2, oe1, oe2, i1, i2, clk):
assert oe1 == oe2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this assert doesn't work right if oe1 and oe2 are a _Slice, like in the litespi ddr phy

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If i remember well, one issue is that the efinix primitive only support 1 output enable signal (in ddr mode).

  • Either when oe1 != oe2 it could fall back to DDR tristate emulation (et get very bad timings)
  • Either it could be ignored and hope for the best
  • Either it can keep the assert

Not sure what is the best.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another options would be to update litespi to avoid the usage of 2 oe signals

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was already with changes like: litex-hub/litespi#60

The problem is this: oe1 = dq_oe[i], oe2 = dq_oe[i], with dq_oe = Signal(len(pads.dq))
where oe1 and oe2 are only a `_Slice``od a Signal and not a whole Signal. the error comes from here: https://github.com/m-labs/migen/blob/832a7240ba32af9cbd4fdd519ddcb4f912534726/migen/fhdl/structure.py#L41

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hoo then would just need to do a dq_oe[i] once before , and refer twice to its result ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should fix it m-labs/migen#292

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants